Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the input cursor position for a selection range #542

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

ShawnCZek
Copy link
Contributor

If the user of the library changes the selection range or the input cursor position, the cursor position is updated but not applied to the scrollable element. This pull request introduces a simple yet effective fix to the problem.

@ShawnCZek ShawnCZek marked this pull request as ready for review November 19, 2023 20:52
@mikke89 mikke89 added the enhancement New feature or request label Nov 21, 2023
@mikke89
Copy link
Owner

mikke89 commented Nov 21, 2023

Yeah, this makes sense to me. However, I can imagine some situations where the user sets the selection without the element being in focus. Then, it shouldn't display the cursor, right? I am not sure whether or not it should move focus?

@ShawnCZek
Copy link
Contributor Author

ShawnCZek commented Nov 22, 2023

That is a very good point. I have not considered that the input element does not have to be focused. We could replicate what most Internet browsers do here; nothing gets selected if the element is not focused. In other words, if the cursor is not shown, SetSelectionRange() early returns. Then, this fix is still valid, but the behavior of the mentioned method must be adjusted.

@mikke89
Copy link
Owner

mikke89 commented Nov 26, 2023

Yeah, I don't see any reasonable use cases for making selection changes on non-focused elements, so that sounds good to me! Do you want to make that change?

Edit: I see you made a separate PR, all good :) I'll wait for that one to be merged first and then this should be good to go.

@mikke89 mikke89 force-pushed the fix_selection_cursor_position branch from 7f63f2d to f281749 Compare November 27, 2023 21:06
@mikke89 mikke89 merged commit c025b63 into mikke89:master Nov 27, 2023
13 of 14 checks passed
@mikke89
Copy link
Owner

mikke89 commented Nov 27, 2023

This one is good to go now, thanks for the PR.

@ShawnCZek ShawnCZek deleted the fix_selection_cursor_position branch November 27, 2023 21:09
alml pushed a commit to alml/RmlUi that referenced this pull request Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants